Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added expected regex for when an optional word may be needed in front… #770

Closed
wants to merge 2 commits into from

Conversation

spatel1009
Copy link

… of alternative group

cucumber-expression-pattern-test

Summary

Added a test for the expected regex pattern for when an optional word may be needed in front of alternative non-capture group.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

No changes or new code has been made towards the app

Checklist:

  • The change has been ported to Java.
  • The change has been ported to Ruby.
  • The change has been ported to JavaScript.
  • The change has been ported to Go.
  • The change has been ported to .NET.
  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@Test
public void translates_alternation_with_optional_words() {
assertPattern(
"the (test )chat/call/email interactions are visible,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a " at the end of this line, so there's no compilation error.

@luke-hill
Copy link
Contributor

The error does look legit. But what's interesting is where it's tripping up is where we half expected it to.

Can you move the space one place back so instead of (capture ) you have ( capture) this should ensure a space between the right ) and the start of the 3 alternated words. This would be a good one to see the output of.

mpkorstanje added a commit that referenced this pull request Oct 24, 2019
Splits the cucumber expression into text, optional, alternative and
parameter tokens. This ensures that rewriting optionals does not
interfere with rewriting alternatives.

Currently processing and splitting is interleaved because the
processing step may transform a token back into a text token again
after handling its escapes.

Fixes: #767
Closes: #770
@spatel1009
Copy link
Author

The error does look legit. But what's interesting is where it's tripping up is where we half expected it to.

Can you move the space one place back so instead of (capture ) you have ( capture) this should ensure a space between the right ) and the start of the 3 alternated words. This would be a good one to see the output of.

Change it to the( test) chat/call/email interactions are visible?

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Oct 24, 2019

It's ok. I found the problem already.

Given any expression of the form (a )c/d.

We first rewrite the optionals to regex so we get (?:a )?c/d.

Then we rewrite the alternatives )?c/d so we get (?:a (?:(?:)?c)|(?:d)) which happens to be a valid regex.

As you can see after rewriting the optionals we rewrite the alternatives. And because the rewritten optionals ends with )? those last two characters are then rewritten into the group for the alternatives.

@luke-hill
Copy link
Contributor

This is really where colours in markdown would help. Took me 15mins to figure out that regex. But yeh @spatel1009 that is right what you said. That should mitigate (But the original post is still causing some friction I agree)

@spatel1009
Copy link
Author

Doing it that way with the( test) call/chat/email will only read a Gherkin written as Then the test call/chat/email.... but not any of those singular like Then the test call.... etc. Also "test" is no longer be optional as steps like Then the chat.... will not be read either.

@luke-hill
Copy link
Contributor

I'm not sure I understand

Then the( test) call/chat/email will match

Then the call
Then the chat
Then the test email

@spatel1009
Copy link
Author

spatel1009 commented Oct 28, 2019

Yes I thought so too but I get "cannot find declaration to go to" when using that above. It reads none of the examples you have listed and the only one it reads is:
Then the test call/chat/email .......
Written exactly like that with all 3 alternative text options written with the /

@luke-hill
Copy link
Contributor

This feels like we're finding more edge cases as we go.

Ok noted! I'll have a little dabble and liaise up with @mpkorstanje to see if we can add tests for this also.

@luke-hill luke-hill added the 🧷 pinned Tells Stalebot not to close this issue label Nov 27, 2019
@mpkorstanje
Copy link
Contributor

Superseded by #771

@mpkorstanje mpkorstanje closed this Jan 1, 2020
mpkorstanje added a commit that referenced this pull request Jul 10, 2020
Splits the cucumber expression into text, optional, alternative and
parameter tokens. This ensures that rewriting optionals does not
interfere with rewriting alternatives.

Currently processing and splitting is interleaved because the
processing step may transform a token back into a text token again
after handling its escapes.

Fixes: #767
Closes: #770
mpkorstanje added a commit that referenced this pull request Jul 17, 2020
Splits the cucumber expression into text, optional, alternative and
parameter tokens. This ensures that rewriting optionals does not
interfere with rewriting alternatives.

Currently processing and splitting is interleaved because the
processing step may transform a token back into a text token again
after handling its escapes.

Fixes: #767
Closes: #770
aslakhellesoy pushed a commit that referenced this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Defect / Bug 🧷 pinned Tells Stalebot not to close this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants